Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Build lib/ objects in the SConscript variant_dir. #1841

Merged
merged 1 commit into from
Oct 24, 2018
Merged

Build lib/ objects in the SConscript variant_dir. #1841

merged 1 commit into from
Oct 24, 2018

Conversation

rryan
Copy link
Member

@rryan rryan commented Oct 14, 2018

SCons has a feature that lets you declare a repository-root absolute
path to a file (e.g. "#lib/soundtouch/..."). Using this feature
prevents SCons from automatically re-directing built artifacts to a
variant_dir. To work around this, this commit moves src/SConscript and
src/SConscript.env to the root, and changes all paths in SConscripts
to use SConscript-relative paths.

This commit also switches to building an explicit static library for
vamp-hostdk, vamp-sdk, and soundtouch when internal linking is enabled
and the VAMP plugin now uses the system soundtouch if it is present.

Fixes Bug #1191327 and Bug #1617802.

@rryan
Copy link
Member Author

rryan commented Oct 14, 2018

Packaging is almost certainly broken, but this is ready for testing by anyone who is interested.

@mixxxdj mixxxdj deleted a comment Oct 14, 2018
@mixxxdj mixxxdj deleted a comment Oct 14, 2018
@mixxxdj mixxxdj deleted a comment Oct 14, 2018
@rryan rryan force-pushed the scons branch 2 times, most recently from 73f8c85 to 6005793 Compare October 14, 2018 07:18
@mixxxdj mixxxdj deleted a comment Oct 14, 2018
@rryan
Copy link
Member Author

rryan commented Oct 14, 2018

Windows packaging is ok -- need to test on Ubuntu and macOS.

@rryan
Copy link
Member Author

rryan commented Oct 14, 2018

macOS packaging is ok.

@rryan rryan changed the title [WIP] Build library objects in the SConscript variant_dir. Build library objects in the SConscript variant_dir. Oct 14, 2018
@rryan
Copy link
Member Author

rryan commented Oct 14, 2018

Ubuntu packaging works.

@rryan
Copy link
Member Author

rryan commented Oct 15, 2018

From a fresh checkout, running scons produces the following files now:

$ git status --ignored           
On branch scons

Ignored files:
  (use "git add -f <file>..." to include in what will be committed)

	.sconf_temp/
	.sconsign.branch
	.sconsign.dblite
	build/__init__.pyc
	build/depends.pyc
	build/features.pyc
	build/mixxx.pyc
	build/protoc.pyc
	build/qt5.pyc
	build/util.pyc
	cache/
	config.log
	lin64_build/
	mixxx

And lin64_build is a mirror of the repo (on a system that has soundtouch and vamp installed):

$ tree -L 2 lin64_build
lin64_build
├── lib
│   ├── fidlib
│   ├── hidapi-0.8.0-rc1
│   ├── libebur128
│   ├── portaudio
│   ├── qtscript-bytearray
│   ├── replaygain
│   ├── reverb
│   └── xwax
├── libmixxx.a
├── mixxx
├── res
│   ├── qrc_mixxx.cc
│   └── qrc_mixxx.o
├── src
│   ├── analyzer
│   ├── broadcast
│   ├── build.h
│   ├── control
│   ├── controllers
│   ├── database
│   ├── dialog
│   ├── effects
│   ├── encoder
│   ├── engine
│   ├── errordialoghandler.o
│   ├── library
│   ├── main.o
│   ├── mixer
│   ├── mixxxapplication.o
│   ├── mixxx.o
│   ├── moc_errordialoghandler.cc
│   ├── moc_errordialoghandler.o
│   ├── moc_mixxxapplication.cc
│   ├── moc_mixxxapplication.o
│   ├── moc_mixxx.cc
│   ├── moc_mixxx.o
│   ├── musicbrainz
│   ├── preferences
│   ├── proto
│   ├── recording
│   ├── skin
│   ├── soundio
│   ├── sources
│   ├── track
│   ├── util
│   ├── vinylcontrol
│   ├── waveform
│   └── widget
└── vamp-plugins
    ├── base
    ├── dsp
    ├── ext
    ├── libmain.os
    ├── libmixxxminimal.so
    ├── maths
    └── plugins

@rryan rryan changed the title Build library objects in the SConscript variant_dir. Build lib/ objects in the SConscript variant_dir. Oct 16, 2018
@benis
Copy link
Contributor

benis commented Oct 17, 2018

Saw your comment on Zulip, building from scratch works fine for me on macOS.

@rryan
Copy link
Member Author

rryan commented Oct 17, 2018

Thanks for testing @beenisss!

@benis
Copy link
Contributor

benis commented Oct 17, 2018

Hmm, I happened to rebuild this via Eclipse just now (used command line previously) and I get this error when it finishes.

screen shot 2018-10-17 at 19 50 38

I don't know if that's significant.

@rryan
Copy link
Member Author

rryan commented Oct 19, 2018

@beenisss I'm not sure what that error indicates on the SCons side. @daschuer do you have any ideas?

@benis
Copy link
Contributor

benis commented Oct 19, 2018

I think you can chalk this up to some transient blip on my local machine, I've been getting some weird inconsistent behaviour with building and cleaning lately. Just tried this again and didn't get the error.

@rryan rryan force-pushed the scons branch 2 times, most recently from d76a031 to 7a92b80 Compare October 20, 2018 17:04
@rryan
Copy link
Member Author

rryan commented Oct 20, 2018

I think this change is ready to go, I've personally tested on windows/macos/linux and @beenisss tested on macos.

@rryan
Build library objects in the SConscript variant_dir.
36e9ca7 
SCons has a feature that lets you declare a repository-root absolute
path to a file (e.g. "#lib/soundtouch/..."). Using this feature
prevents SCons from automatically re-directing built artifacts to a
variant_dir. To work around this, this commit moves src/SConscript and
src/SConscript.env to the root, and changes all paths in SConscripts
to use SConscript-relative paths.

This commit also switches to building an explicit static library for
vamp-hostdk, vamp-sdk, and soundtouch when internal linking is enabled
and the VAMP plugin now uses the system soundtouch if it is present.

Fixes Bug #1191327 and Bug #1617802.
@rryan
Copy link
Member Author

rryan commented Oct 24, 2018

Any objections? I'd like to merge this so I can stop having to rebase :).

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2018

Will test soon

Be-ing
Be-ing reviewed Oct 24, 2018
View reviewed changes
SConscript.env
if os.path.exists(defs):
print("Deleting deprecated build file: %s" % defs)
os.remove(defs)
# We are in the variant_dir (win|mac|lin)XX_build, and the 'src' subdirectory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason we put the OS name in the build directory name? Why not just call the directory build like is conventional with CMake?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, I think if you switch between 32-bit and 64-bit builds on the same checkout it could be handy and @Pegasus-RPG has done that in the past. But I don't think the OS name needs to be there.

@Be-ing
Copy link
Contributor

Be-ing commented Oct 24, 2018

Works for me 👍

@Be-ing Be-ing merged commit 297d880 into mixxxdj:master Oct 24, 2018
@rryan
Copy link
Member Author

rryan commented Oct 24, 2018

Works for me 👍

Thanks for testing!

@Be-ing Be-ing mentioned this pull request Nov 30, 2018
Merged
This was referenced Aug 22, 2022
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Be-ing Be-ing Be-ing left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rryan @benis @Be-ing